Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Q for Native Promise #78

Merged
merged 2 commits into from Jun 18, 2019
Merged

Remove Q for Native Promise #78

merged 2 commits into from Jun 18, 2019

Conversation

erisu
Copy link
Member

@erisu erisu commented Jun 14, 2019 •
edited

Platforms affected

browser

Motivation and Context

Remove Q Efforts: apache/cordova#7

Description

This repo did not have the Q dependency defined explicit in package.json but was still using Q that would have been a sub-dependency of cordova-common.

This PR removes the only usage of Q.

Testing

  • npm t

  • Update with Good Project Path

$ ./cordova-browser/bin/update /cordova/projectPath
Removing existing browser platform.
Successfully updated browser project.
  • Update with Bad Path
$ ./cordova-browser/bin/update /cordova/fakepath
Update failed due to CordovaError: Browser platform does not exist here: /cordova/fakepath_
    at Object.module.exports.run (/cordova/cordova-browser/bin/lib/update.js:38:31)
    at Object.<anonymous> (/cordova/cordova-browser/bin/update:27:12)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:837:10)
    at internal/main/run_main_module.js:17:11 {
  name: 'CordovaError',
  message: 'Browser platform does not exist here: ' +
    '/cordova/.testsss_',
  code: 0,
  context: undefined
}

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@erisu
Copy link
Member Author

erisu commented Jun 14, 2019 •
edited

Warning

I want to point out that this update script never checked or verified the provided project path.
If the path is incorrect but does exist, it will still wipe out the contents of that folder. So, do not accidentally put in the wrong folder or you might end up loosing something you did not intend to delete.

@brodybits
Copy link

I want to point out that this update script never checked or verified the provided project path.
[...]

That does not sound so good. Unfortunately I cannot tell for sure what is the corresponding behavior on the other platforms.

I would favor raising one or more issues to track both the behavior and level of consistency between the supported platforms.

I think it should be OK to continue with this change now, and deal with that behavior whenever we can.

I cannot tell whether or not this may be a breaking change. In case of any doubt, I would favor targeting this change for the next major release.

And a nit that I do not favor unchecked items in the checklist. I personally do strikethrough on the items that are not needed, not sure if we have any consensus about this.

@erisu
Copy link
Member Author

erisu commented Jun 14, 2019 •
edited

I also agree that this does not sound good. Fixing the behavior or discussing the removal of this process can be separate issue ticket, as you pointed out. It might be easier if users just remove and add back the platform.

I also agree that we can continue on as the purpose of this PR is achieved. The issue I raised would be out of scope from this ticket. It just happened to have been discovered while working on this.

Anyways, I feel that this is not a major change and could be a minor release. I did not remove dependencies as it never existed in the package.json.

Also, I believe these changes only affect non-CLI workflow. I didn't see update.js required in the CLI workflow.

  • Does anyone else see it used in the CLI workflow?
  • Anyone else has an opinion on the target release?

raphinesse
raphinesse approved these changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bin/update Outdated Show resolved Hide resolved
@raphinesse
Copy link
Contributor

Regarding fixing the update script: IIRC, cordova-android and cordova-ios do not even support update. If we want to provide an update mechanism, we should probably just implement a generic delete/create in the CLI and remove it from the platforms altogether.

@erisu @raphinesse
Update bin/update
f3f1693 
Co-Authored-By: Raphael von der Grün <raphinesse@gmail.com>
@erisu
Copy link
Member Author

erisu commented Jun 18, 2019

@raphinesse I never tried the CLI route. What I tested was the non-CLI, Platform-centric, use cases. If the Platform-centric use cases were removed, then we could simplify and make a generic remove and add process.

@erisu erisu merged commit 9a5eddc into apache:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphinesse raphinesse raphinesse approved these changes

@dpogue dpogue Awaiting requested review from dpogue

@brodybits brodybits Awaiting requested review from brodybits

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@erisu @brodybits @raphinesse